pull up c-band prefix decoding #174
Conversation
|
|
||
| // https://app.airframes.io/messages/3452310240 | ||
| const text = '101606,1910,.N317FR,,KMDW,----,1,1016,RT0,LT1,'; | ||
| const decodeResult = decoderPlugin.decode({ text: text }); | ||
| const decodeResult = decoder.decode({ label: "4A", text: text }); |
There was a problem hiding this comment.
this isn't a bad idea, but it's going to make the PR huge. Do we want a separate PR that just makes this change? Or do we want to move the c-band messages to the MessageDecoder suite? I'm leaning towards moving the c-band tests
There was a problem hiding this comment.
Moving the c band message tests makes sense with two drawbacks:
- Need to remember to add tests in a second location for types with this prefix
- More importantly, there's no way to check if the cband prefix accidentally catches non cband messages and incorrectly truncates the message that it passes into a plugin.
There was a problem hiding this comment.
do we have messages (minus the prefix) that are unique to c-band? If not, i don't think we need to have c-band label-specific tests other than a couple to test the prefix extraction.
As for the second point, idk. Is it worth slowing down the test suite by making every test run through the plugin selection code just so we can verify this edge case that we don't even know exists?
There was a problem hiding this comment.
not sure about any cband specific messages, but the regex for the prefix is pretty permissive. i would be surprised if it never matched any non cband messages. maybe it would be better to send more metadata about the source of the message into the decoders and just tell it explicitly if it's cband?
or we could try to decode as cband if the prefix is detected then retry with the prefix still attached if it fails to decode. that wouldn't necessarily catch every edge case though
There was a problem hiding this comment.
@kevinelliott and @andermatt64 - Do you have any opinions here?
rpatel3001
force-pushed
the
cband_prefix
branch
2 times, most recently
from
84e959e to
cf15587
Compare
makrsmark
force-pushed
the
master
branch
3 times, most recently
from
3447c21 to
1e877a4
Compare
rpatel3001
force-pushed
the
cband_prefix
branch
2 times, most recently
from
ad6e34a to
da1033b
Compare
rpatel3001
force-pushed
the
cband_prefix
branch
from
da1033b to
fe1cdc7
Compare
|
@rpatel3001 @andermatt64 @makrsmark Any further thoughts here? It would be good to get this one finished up. |
I think I'm ok with the airline+number regex if historical C-band messages can be consistently caught with it. |
|
last thing to resolve is the conversation about how to do c-band tests: make each plugin test use |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request enhances the message decoding process by updating the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant MD as MessageDecoder
participant RX as Regex Parser
participant RF as ResultFormatter
C->>MD: decode(message)
MD->>RX: Check for 10-character header
alt Header Matched
RX-->>MD: Return extracted groups (message number, flight number)
MD->>RF: flightNumber(airlineCode, flightNumber)
MD->>MD: Adjust text with header restored
else No Header
RX-->>MD: No match found
MD->>MD: Process message normally
end
MD-->>C: Return decoded message
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/MessageDecoder.ts(3 hunks)lib/plugins/Label_1L_Slash.ts(1 hunks)lib/plugins/Label_30_Slash_EA.test.ts(1 hunks)lib/plugins/Label_4A.test.ts(6 hunks)lib/plugins/Label_4A.ts(2 hunks)lib/plugins/Label_4N.test.ts(5 hunks)lib/plugins/Label_4N.ts(2 hunks)lib/plugins/Label_83.test.ts(6 hunks)lib/plugins/Label_83.ts(2 hunks)
🔇 Additional comments (20)
lib/plugins/Label_1L_Slash.ts (1)
83-83: Good formatting change.Adding a newline at the end of the file is a good practice that helps meet common coding standards and avoids "No newline at end of file" warnings from linters.
lib/plugins/Label_4A.ts (2)
24-24: Improved code by removing intermediate variable.Directly using
message.text.split(",")instead of an intermediate variable simplifies the code and aligns with how the other decoders have been refactored.
62-62: Consistent use ofmessage.text.Using
message.textdirectly in the error case maintains consistency with other changes and the decoder pattern being implemented throughout the codebase.lib/MessageDecoder.ts (3)
5-5: Required import added for the new functionality.The
ResultFormatterimport is necessary for the new C-Band prefix handling that usesResultFormatter.flightNumber().
99-105: Well-implemented C-Band prefix detection and extraction.The implementation follows a clear pattern:
- Identify messages with a 10-character C-Band header using a descriptive regex
- Extract the message number, airline code, and flight number
- Temporarily remove the prefix from the message text for downstream processing
The regex pattern
^(?<msgno>[A-Z]\d{2}[A-Z])(?<airline>[A-Z0-9]{2})(?<number>[0-9]{4})captures the structure documented in previous discussions about C-Band prefixes.
162-165: Good restoration and enrichment of message data.After processing the message with plugins, the implementation:
- Properly checks if a C-Band prefix was detected
- Adds the flight number information using
ResultFormatter.flightNumber()- Restores the original message text with the prefix
This approach preserves the original message while enriching the decoded result with flight information.
lib/plugins/Label_30_Slash_EA.test.ts (1)
17-17: Test updated to use MessageDecoder instead of plugin instance directly.This change aligns with the PR objective of using a top-level
MessageDecoderinstance across all tests. The test now passes both thelabelandtextto the decoder, which better matches the actual usage pattern in the application.lib/plugins/Label_4N.test.ts (3)
17-17: Refactoring to use MessageDecoder directlyThis change aligns with the PR objective of using a top-level
MessageDecoderinstance for tests instead of individual plugin instances. This approach is more consistent with how the decoder would be used in production.
42-42: Consistent use of MessageDecoder across all test casesAll test cases have been updated to use the
MessageDecoderinstance directly, providing a consistent testing approach throughout the file.Also applies to: 75-75, 102-102, 133-133
113-126: Updated expectations for C-band message formatThe order of the formatted items has been adjusted, with the flight number now appearing as the last item in the array. This change reflects the new structure where the MessageDecoder handles C-band prefix decoding rather than the individual plugin.
lib/plugins/Label_83.ts (1)
23-23: Direct use of message.text instead of intermediate variableThe code now directly references
message.textinstead of using a local variable. This simplification is cleaner and reduces unnecessary variable assignments, which is a positive change that helps maintain consistency across plugins.Also applies to: 25-26, 35-35, 37-38, 42-43
lib/plugins/Label_4N.ts (1)
22-31: Simplified message text handlingSimilar to the changes in Label_83.ts, this plugin now directly uses
message.textinstead of an intermediate variable. This simplification makes the code more straightforward and aligns with the architectural change to move C-band prefix handling to the MessageDecoder.Also applies to: 49-49
lib/plugins/Label_83.test.ts (2)
17-17: Consistent testing methodology using MessageDecoderAll test cases now use the
MessageDecoderinstance directly for decoding, passing both the label and text. This approach is more consistent with real-world usage and aligns with the PR objective.Also applies to: 53-53, 92-92, 121-121, 145-145, 171-171
71-85: Updated item order in formatted results for C-band messagesThe expectations for C-band message test cases have been updated to reflect the new structure where flight number appears as a separate item. This is consistent with the changes in Label_4N.test.ts and reflects the architectural change where the MessageDecoder handles C-band prefix decoding.
Also applies to: 159-165
lib/plugins/Label_4A.test.ts (6)
21-21: Methodology update aligns with PR objectivesThis change moves from direct plugin invocation to using the top-level
MessageDecoder, aligning with the PR's goal of centralizing the C-band prefix decoding.
47-47: Centralized decoder approach matches PR objectivesSwitching to the top-level decoder instance is consistent with the discussions in previous reviews about improving the C-band prefix handling.
55-55: Ensure item count reduction is intentionalSimilar to line 29, the expected item count has been reduced from 5 to 4. This appears to be related to how the
MessageDecodernow processes these messages.
71-71: Consistent methodology across all test casesThe change to use the top-level decoder is applied consistently across all test variants, which is good for maintainability and aligns with the PR objectives.
Also applies to: 95-95, 121-121
29-29:✅ Verification successful
Verify the expected item count reduction
The expected length of formatted items has been reduced from 6 to 5. This change appears to be related to how the message is now being processed by the central decoder.
🏁 Script executed:
#!/bin/bash # Check if any similar tests were modified to reduce expected item counts grep -n "expect.*formatted.items.length" lib/plugins/*.test.ts | grep -B 1 -A 1 "toBe"Length of output: 16474
Central Decoder Update: Expected Item Count Reduction Confirmed
The change in expected item count—from 6 to 5—in
lib/plugins/Label_4A.test.ts(line 29) is consistent with the updates observed across similar tests in the repository. The grep results confirm that the new expected value (5) reflects the revised central decoder processing logic. No additional changes are needed here.
104-114:✅ Verification successful
C-Band flight information now appears at the end
The order of formatted items in the C-Band test case has changed, with the 'FLIGHT' item now placed at the end of the array. This likely reflects how the central decoder now processes C-Band messages with flight information.
🏁 Script executed:
#!/bin/bash # Check how the MessageDecoder is handling flight information for C-Band messages cat lib/MessageDecoder.ts | grep -A 30 "decode" | grep -A 30 "flight"Length of output: 1814
Flight Information Order Change Validated
The updated test snapshot in
lib/plugins/Label_4A.test.tsnow correctly expects the flight information (FLIGHT) to appear as the last item in the formatted items array. Verification via theMessageDecoder.tssnippet confirms that the flight number is appended—via theResultFormatter.flightNumbercall—after processing the other fields, which aligns with the expected behavior.No further modifications are required.
| // expect(decodeResult.decoded).toBe(false); | ||
| // expect(decodeResult.decoder.decodeLevel).toBe('none'); | ||
| expect(decodeResult.decoder.name).not.toBe('label-4a'); | ||
| // expect(decodeResult.formatted.description).toBe('Latest New Format'); | ||
| // expect(decodeResult.formatted.items.length).toBe(0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update or restore the commented assertions
Several assertions have been commented out rather than updated. To ensure complete test coverage, consider either:
- Updating these assertions to match the new expected behavior, or
- Adding new assertions that verify the correct behavior under the new decoding approach
The current approach may leave some aspects of the invalid case handling untested.
-// expect(decodeResult.decoded).toBe(false);
-// expect(decodeResult.decoder.decodeLevel).toBe('none');
+ expect(decodeResult.decoded).toBe(false);
+ // If decoding behavior has changed, update expected values:
+ expect(decodeResult.decoder.decodeLevel).toBe('none');
expect(decodeResult.decoder.name).not.toBe('label-4a');
-// expect(decodeResult.formatted.description).toBe('Latest New Format');
-// expect(decodeResult.formatted.items.length).toBe(0);
+ // Add appropriate assertions for the new decoding approach
+ expect(decodeResult.formatted.items.length).toBe(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // expect(decodeResult.decoded).toBe(false); | |
| // expect(decodeResult.decoder.decodeLevel).toBe('none'); | |
| expect(decodeResult.decoder.name).not.toBe('label-4a'); | |
| // expect(decodeResult.formatted.description).toBe('Latest New Format'); | |
| // expect(decodeResult.formatted.items.length).toBe(0); | |
| expect(decodeResult.decoded).toBe(false); | |
| // If decoding behavior has changed, update expected values: | |
| expect(decodeResult.decoder.decodeLevel).toBe('none'); | |
| expect(decodeResult.decoder.name).not.toBe('label-4a'); | |
| // Add appropriate assertions for the new decoding approach | |
| expect(decodeResult.formatted.items.length).toBe(0); |
|
revisiting this, I think I agree that adding c-band tests to each message type is perhaps too much and so it would be better to have a c-band specific test file. i can pull them out and revert the existing tests in the next couple days |
doing it this way means using top level MessageDecoder instance in all the tests instead of an instance of the specific plugin being tested. if that's ok i can update the rest to match (and check if the C-Band prefix test matches any messages it shouldn't).
Summary by CodeRabbit
New Features
Refactor